Skip to content

[In-app Purchases] Improve transaction handling logic #8124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Nov 16, 2022
Merged

[In-app Purchases] Improve transaction handling logic #8124

merged 4 commits into from
Nov 16, 2022

Conversation

@koke
Copy link
Member

@koke koke commented Nov 15, 2022 •
edited
Loading

Closes: #8121

Description

This PR intends to improve some of the transaction handling logic:

  • First, by pausing the transaction listener while making a purchase, so the purchase logic gets to create the order first. I've observed that sometimes there were race conditions where the transaction update got there first and the purchase failed because it was trying to create a duplicate order. This is a bit random and hard to reproduce though.
  • Then, even if we re-send a transaction, if the backend returns a product_purchased error, it means that we have already sent that transaction before and there's an order for it. So now we treat this case as a success instead of a failure

Testing instructions

  1. Go to Hub menu > IAP Debug
  2. Purchase a new subscription
  3. Notice the Pausing transaction listener and Resuming transaction listener messages in the console while the purchase is happening
  4. Try to purchase the same product again. iOS will tell you that you already own that, but after dismissing the dialog, it returns a transaction and the app tries to submit that
  5. Notice the Sending transaction to API message in the console followed by Existing order found for transaction ##### on site #####, ignoring. The app should show no alert with an error (unless the API call failed for another reason)
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@koke koke added this to the 11.3 milestone Nov 15, 2022
@koke koke added type: task An internally driven task. feature: in-app purchases Related to In-app purchases and subscriptions labels Nov 15, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 15, 2022 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8124-d3945c7 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@koke koke requested a review from toupper November 15, 2022 16:09
@koke koke marked this pull request as ready for review November 15, 2022 16:09
@toupper
Copy link
Contributor

toupper commented Nov 15, 2022

Code looks good and tests well, just a small question there.

toupper
toupper reviewed Nov 15, 2022
View reviewed changes
Yosemite/Yosemite/Stores/InAppPurchaseStore.swift
do {
try await self?.handleCompletedTransaction(result)
// Wait until the purchase finishes
_ = await self.pauseTransactionListener.values.contains(false)
Copy link
Contributor

@toupper toupper Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this approach it can happen that we handle the same transaction twice right, sending a duplicated order? Even if the backend ignores duplicated requests, it would be nice to prevent sending the same requests twice.

Copy link
Member Author

@koke koke Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is fine. Since we aren't using the App Store Server API v2 yet, we aren't really sending "a transaction" to the API. We send the app receipt, which might not have even changed between requests, and use Apple's verifyReceipt endpoint response to get the list of transactions.

I think dealing with potential duplicate requests is something we need to live with, and this PR doesn't change that. It only ensures that if a duplicate transaction happens, it happens in the right order.

Copy link
Contributor

@toupper toupper Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your answer @koke. It's a pity that we cannot prevent at the moment sending duplicate requests in a simple way for now, but we can live with that.

@toupper
Copy link
Contributor

toupper commented Nov 16, 2022

Looks good to me and tests well, only that comment that we talked about. Thanks for this @koke! Now the whole implementation is much more robust 🎉 :shipit:

@koke koke merged commit 472ec8b into trunk Nov 16, 2022
@koke koke deleted the issue/8121-improve-transaction-listener branch November 16, 2022 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toupper toupper toupper approved these changes

Assignees

No one assigned

Labels

feature: in-app purchases Related to In-app purchases and subscriptions type: task An internally driven task.

Projects

None yet

Milestone

11.3

Development

Successfully merging this pull request may close these issues.

[In-app Purchases] Improve transaction listener logic

4 participants

@koke @wpmobilebot @toupper